Providing a way to set traitlets with environment variables is done in the kernel gateway code base, but it's not common in the notebook codebase.

Let's simplify the trait declarations for now. @parente is right that the environment variable declarations aren't generally used here, so we can have a single assignment:

cull_idle_timeout = Integer(0, config=True,
    help="...
)

without the @default generator or other variables defined.

Plus, I think it might be clearer to use seconds everywhere, rather than a _minutes timeout here and a _seconds timeout below. How about we call them:

cull_idle_timeout: timeout (in seconds) after which a kernel is considered idle and ready to be culled.
cull_interval: interval (in seconds) on which to check for idle kernels to cleanup...

Thank you - this has been addressed in the subsequent commit.

cull_kernel can just take a kernel_id argument, and the above can be for kernel_id in list(self._kernels). The list will make a copy of the keys, which may be needed, as shutting down a kernel will remove it from the _kernels dictionary and you can't iterate over a dictionary while changing it, for example:

for kernel_id in list(self._kernels):
    self.cull_kernel_if_idle(kernel_id)

def cull_kernel_if_idle(self, kernel_id):
    kernel = self._kernels[kernel_id]
    ...

Good point - thanks.

log.warn is deprecated in Python in favor of log.warning

+1

In Python, we try to use _ to separate words, such as kernel_id, idle_time instead of camelCase

I had noticed the convention previously but am finding this to be a very difficult habit to break - thanks for pointing it out.

Should we also warn for cull_interval < 60s ? it's technically not wrong, but it's likely that user might have thoughs it was minutes. Even anything below 300s seem it can be a mistake.

Yeah, I agree. I thought implementing a minimum timeout would be good (now that I've tested it :-)). 300 strikes me as good minimum. If folks are making changes in these areas and need shorter timeouts for testing, they can temporarily adjust the constant.

Positive values less than the minimum would result in a warning and auto-adjustment to the minimum (similar to what happens if the interval is <= 0 and culling is enabled).

So, to your comment above, the description would be...

"Timeout (in seconds) after which a kernel is considered idle and ready to be culled. Values of 0 or lower disable culling. The minimum timeout is 300 seconds. Positive values less than the minimum will be adjusted to the minimum."

(or is that too verbose?)

@Carreau a warning is a good idea.

@kevin-bates that sounds perfect. You might add (5 minutes) next to 300 seconds.

Will do.

Nitpicking, I think a "Values of 0 or lower will disable culling" could be nice. But we can improve later in a subsequent PR.

+1

I'm guessing the list() here is to reify the things you want to try to cull and make sure you don't modify the list while culling. Is that right ? If so it seem like something easily broken.

What do @minrk and @parente think about building the list of kernel to shutdown and shut them al down at once ?
That should make the flow a little bit easier to follow (and also to change the predicate if necessary).

Either way is fine. I don't think getting a list of dictionary keys is easily broken.

Would this be clearer?

# identify the idle kernels to be culled
to_cull = [ kid for kid in self._kernels if self.should_cull(kid) ]
# cull them
for kernel_id in to_cull:
    self.shutdown_kernel(kernel_id)

What do @minrk and @parente think about building the list of kernel to shutdown and shut them al down at once ?

Sounds good to me as long as there's appropriate checks in place so that an problem getting the status of any one kernel (if that's possible) doesn't prevent cleanup of others that should be culled.

I guess I'm not really following the benefit of obtaining the list of cullable kernels, then walking that list to perform the cull when we could just as easily encounter an issue determining the cullable kernels. So, to address the point raised by @parente, what about something like the following? (Is that the right way to handle an unknown set of exceptions in python?)

        for kernel_id in list(self._kernels):
            try:
                self.cull_kernel_if_idle(kernel_id)
            except:
                self.log.error("The following error was encountered while checking the idle duration of kernel %s: %s",
                    kernel_id, sys.exc_info()[0])

That works, too. There are plenty of valid ways to do it. A few notes:

• a comment about copying the kernel id list so we don't modify the dict during iteration might help clarity
• you can use log.exception to automatically log a traceback, so you don't need to use exc_info.
• always use at least except Exception:, because except: catches things like KeyboardInterrupts that shouldn't be caught.

Thank you @minrk - very helpful. Here's what I've got...

        """Create a separate list of kernels to avoid conflicting updates while iterating"""
        for kernel_id in list(self._kernels):
            try:
                self.cull_kernel_if_idle(kernel_id)
            except Exception as e:
                self.log.exception("The following exception was encountered while checking the idle duration of kernel %s: %s",
                    kernel_id, e)